-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move/copy mechanisms #8
Conversation
`vertices` field should be set to `nullptr`, because it will be used in the destructor and values other than `nullptr` can lead to undefined behaviour
Hi! Looks like our conversation with Codacy support has finished. They verified that this was an issue on Codacy side (fix of which is already committed but not yet deployed to production) and suggested ignoring the issues for now. The PR is absolutely ready for a review! |
…operators No idea why I have written them. They look redundant and even strange in somme places
acaf60c
to
40b94e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Found some issues and left comments
src/Face.cuh
Outdated
__device__ Face &operator=(Face &&other) noexcept; | ||
|
||
/** | ||
* Face` object move constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be back quote before Face`
{ | ||
if(this != &other) | ||
{ | ||
vertices = malloc_and_copy(other.vertices, other.n_of_vertices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field Face::vertices
has type const SpacePoint
. Other variables which were moved to private section of their classes have no const
in their types or const
was removed from the type. Does Face::vertices
really need const in type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, note, that the actual type is const SpacePoint *
(*
is an important part). It is different from the const SpacePoint *const
type (which would mean the field vertices
can't be modified), but the actual time means that the content of the array can't be modified without replacing the whole array. If the field was const pointer, it would take away the opportunity to, for example, move a face into an existing one, but the pointer to const only prevents us from accidentally modifying the vertices array (vertices of an existing Face
shouldn't change i. e. have a specific set of vertices, shouldn't they?). Because the field is private, it's already impossible to modify the array from outside of the Face
class, but is still possible to modify it from inside, which shouldn't happen and is worth protecting.
The general rule is if you don't modify a variable, make it constant. This way you will get additional protection from your compiler
@@ -46,9 +67,6 @@ public: | |||
/// Pointer-represented array of nodes on the map | |||
MapNode *nodes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has just drawn my attention, it might be not the problem of this PR. I suppose field SimulationMap::nodes
should be moved to private, because it should not be changed after its creation in SimulationMap
constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a separate issue
src/Polyhedron.cu
Outdated
__device__ Polyhedron &Polyhedron::operator=(Polyhedron &&other) noexcept | ||
{ | ||
if(this != &other) | ||
{ | ||
faces = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polyhedron
B has to be moved to Polyhedron
A. If A was created earlier (move constructor is not called during the moving), then setting A.faces
to nullptr
will cause leak of memory that A.faces
contained before moving B to A. Same to Face::vertices
, MapNode::particle
(I suppose) and SimulationMap::nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right (by the way, I think, your explanation could be easier to understand if you'd provide a code snippet with an example). And yes, this does work for all Polyhedron::faces
, Face::vertices
, MapNode::particle
and SimulationMap::nodes
. Thank you for the alarm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
# Conflicts: # src/MapNode.cu
Added or restricted move/copy mechanisms for classes. Fixes #3
Is blocked by #6. The current branch (move_copy_mechanisms
) is based onrefactoring_face_class
, so the corresponding PR should be approved before approving this, and new commits atrefactoring_face_class
should be merged into the current branch if there are any